test(cmd): assert what commands actually send to the API - #65
Merged
Conversation
aaearon
force-pushed
the
test/argument-capture
branch
2 times, most recently
from
August 16, 2026 13:31
b69451d to
5eeae91
Compare
Merge argument capture into mockAccessRequestService and delete the capturing variant, and record request histories for mockElevateService and mockGroupsElevator. Move withInteractiveTTY into the shared helpers file, renamed to _test.go so test-only code stops linking into the binary.
Cover the finalize decision, cancel/get request IDs, list params, submit payload, the validateSubmitFields call site, the non-TTY guards on get/approve/reject, the CSP-only GCP rejection, and login's auto-configure branch and authenticate flags.
Add argument and guard coverage for elevateCloud/elevateGroup/resolveAndElevate payloads, the four grant env --favorite paths, ErrorInfo handling, the three empty-results guards, total multi-CSP failure, env's fresh post-prompt context, the auth cache flag, selector input and --group/--groups precedence. Extract shouldShowVerboseHint so the hint condition is testable without restating it, drop a duplicated multi-CSP case and widen the concurrency bound.
All 42 argument-capture rows reverified fail-to-pass with -count=1; records the mutation forms used where the literal form does not compile.
Extracting shouldShowVerboseHint made the predicate testable but left the call site unpinned: executeWithHint calls the same predicate, so deleting the whole if-block from Execute() kept go test ./cmd/ green. TestIntegration_VerboseHint drives the compiled binary and asserts the hint is present on a runtime error and absent for an unknown subcommand, an unknown flag, and an already-verbose run.
TestRunRequestSubmit_MissingFlags_NonInteractive relied on go test's ambient non-TTY stdin, the one violation of the MUST added in PR4. It fails under a forced TTY; withInteractiveTTY(t, false) makes the precondition its name already claims an assertion. Also switches the GCP negative assertion to len(submitCalls) != 0, which distinguishes 'never called' from 'called with nil'.
TestRunLogin_AutoConfiguresMissingProfile reaches runConfigure, and survey writes prompts straight to os.Stdout rather than the cobra buffer, so every go test ./cmd/ emitted control sequences. One of them, ESC[6n, makes the terminal reply on stdin and corrupt the shell prompt. withDiscardedStdout redirects os.Stdout for the test's duration, restoring it via t.Cleanup, with no production prompt seam.
The CLAUDE.md note claimed the old production cmd/test_helpers.go 'linked test-only code into the binary'. It did not: that file imported only bytes and cobra, and 'go list -deps . | rg testing' matches on neither the base nor HEAD. The move was preventive. The two remaining lastSubmit() != nil negatives become len(submitCalls) != 0, matching the elevateCalls form already used in the env tests and distinguishing 'never called' from 'called with nil'.
cmd/test_mocks.go was 443 lines of test scaffolding compiled into the production cmd package. The same argument that moved test_helpers.go applies with more force here: this is the larger and faster-growing of the two files. go list -deps . | grep -c '^testing$' stays 0.
…mbers Extracting shouldShowVerboseHint shifted everything below it by ten lines, so every cmd/root.go:NNN reference written in this PR pointed at unrelated code. The no-mutex safety argument is carried entirely by those references, so a reviewer following them landed nowhere useful. Replaced with function names, which do not rot.
…llapse - TestRunRequestSubmit_SubmitPayload becomes a table over Azure and AWS. The AWS row is where locationType stops being a naive string(ws.CSP), so an Azure-only fixture left "AWS" -> "Aws" alive. - The RequestDetails assertion now compares key counts, so an extra key in the payload fails instead of going unnoticed. - Add a --reason "" case to the cancel and finalize arg tests, pinning the deliberate collapse of explicit-empty into unset. - lastX() accessors return a pointer to a copy, not into the history slice's backing array, which append can reallocate under a held pointer.
COV-01: deleting the verbose-hint call site from Execute() is killed only on the integration leg; the default local go test ./cmd/ loop leaves it green. COV-02: TestRunLogin_AutoConfiguresMissingProfile skips under a PTY, so the auto-configure branch has no coverage there. survey reads os.Stdin with no injectable seam, so this is accepted rather than fixed; the skip comment now says so plainly.
aaearon
force-pushed
the
test/argument-capture
branch
from
August 16, 2026 13:36
4c4e821 to
0c5806d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 4 of 8. Base:
test/isolation-harness(#63) — must merge first.Why
The shared mocks in
cmd/test_mocks.godiscarded their arguments, so tests asserted against canned mock return values rather than what the command sent. Verified consequences that shipped green:grant request rejectcould sendAPPROVED— the tests only checked the printed word, which the command generates locally.--state PENDINGcould list everything;--descinverted;--searchdropped.WorkspaceID/RoleIDor blank the CSP and org ID.validateSubmitFieldscall site was undetectable — the validator was only tested in isolation.What
Capture merged into
mockAccessRequestService(the capturing subclass is deleted), plusmockElevateServiceandmockGroupsElevator. History slices withlastX()accessors, defensive copies, andreason string+reasonSet boolsonilis distinguishable from"".No mutex, deliberately. Only the eligibility listers fan out —
fetchEligibility/fetchGroupsEligibilityandresolveAndElevateUnifiedPathinroot.go,fetchAllTargets/fetchAllGroupsinhelpers.go— and those channels are joined before any elevate path is reached, so concurrent access to a capturing mock is unreachable, not merely unobserved. Independently re-derived in review, plus-race -count=20and-race -shuffle=on -count=5clean. Cited by function name rather than line number: the original line-number form was invalidated by theshouldShowVerboseHintextraction in this same PR.Also closes:
findItemByDisplayreturning the wrong item (silent wrong-target elevation), the fourenv --favoritepaths, group/envErrorInfohandling (a POLICY_DENIED result printed success and exited 0), three empty-results guards, total multi-CSP failure, and at.Skipwhose stated reason was factually wrong.Notes
cmd/test_helpers.gowas a production file; addingwithInteractiveTTYto it would have importedtestinginto the shipped binary. Moved to_test.go.cmd/test_mocks.gowas production too — 443 lines that this PR grew by 151 — so it moved tocmd/test_mocks_test.gofor the same reason, with more force.go list -deps . | grep -c '^testing$'→ 0 on both base and head.shouldShowVerboseHintmadeexecuteWithHintunable to disagree withExecute(), so deleting the hint block passed the unit suite. Pinned from the integration side instead — and the ledger records that dropping-tags=integrationfrom CI would unpin it again.go test's stdin happens not to be a TTY. Now explicit.Deletions, disclosed
"multi-CSP concurrent fetch - parallel execution"case incmd/root_elevate_test.gois deleted, andTestFetchEligibility_ConcurrentExecution's bound is loosened from 350ms to 500ms (three CSPs x 200ms: sequential ~600ms, concurrent ~200ms, so the bound still separates the two regimes; no flake was observed at the old bound, and none is claimed). The coverage is genuinely replaced, not dropped — mutating the multi-CSP fan-out still dies viaTestRootElevate_InteractiveModeandTestFetchEligibility_AllProvidersIncludesGCP. Recorded as ledger rows ELV-26/27.Coverage caveats, disclosed
Both are recorded as ledger rows so they cannot be quietly lost:
if shouldShowVerboseHint(...) { ... }block fromExecute()leavesgo test ./cmd/green. It is killed only under-tags=integration. CI runs that leg unguarded on both OSes, so the call site is covered; the default localmake testloop is not.TestRunLogin_AutoConfiguresMissingProfileskips under a PTY, so the auto-configure branch has zero coverage there.runConfigureprompts throughsurvey, which readsos.Stdinwith no injectable seam, so on a real terminal the test would block rather than fail. Accepted, not fixed; the skip comment states the gap plainly.42 ledger rows, all mutation-reverified. Adversarial review performed (Codex credits exhausted; review by a Claude agent). All findings fixed.